Review: fix: sync-main-to-experimental action correct base branch

Files changed: 1 (+67 −5) — .github/workflows/sync-main-to-experimental.yml
CI: All checks passing ✅
ADR-6: PR title (fix: …) and branch (fix/sync-main-to-experimental-action) follow semantic conventions ✅

Problem & Fix Assessment

The old workflow was fundamentally broken: git checkout -B chore/sync without specifying a start point created chore/sync at main's HEAD, discarding all experimental-only content. The new approach correctly builds chore/sync as experimental + main — experimental-only protos survive the sync.

Analysis

✅ Architecture & Merge Strategy

• Correct base: branching from origin/experimental (or reusing origin/chore/sync when a PR is already open) ensures experimental-only content is preserved.
• Uniform merge loop: the for REF in origin/experimental origin/main loop handles both fresh and reuse paths cleanly. When starting from origin/experimental, the first merge is a no-op ("Already up to date") — correct and harmless.
• Conflict handling: merge --abort || true + ::error:: + exit 1 fails cleanly instead of pushing broken state.
• No-op detection: git diff --quiet origin/experimental HEAD correctly skips the push and PR creation when experimental already contains everything in main.

✅ Race Condition Prevention

• Concurrency group (cancel-in-progress: false) ensures two pushes to main queue instead of racing on the push to chore/sync.
• --force-with-lease instead of --force guards against overwriting manual conflict resolutions pushed between the fetch and the push. Significant safety improvement over the old --force.

✅ Edge Cases Verified

✅ Security

• No secrets exposure: only GITHUB_TOKEN (automatically provisioned).
• Minimal permissions: contents: write (push), pull-requests: write (PR create/comment), issues: write (needed by gh pr comment/--label which internally hit the Issues API). All justified.
• No injection vectors: no untrusted user input flows into shell commands. The workflow triggers are push (to main) and workflow_dispatch — both trusted.
• No hardcoded credentials or sensitive data in logs.

✅ Shell Scripting Quality

• set -euo pipefail on all run blocks.
• Variables properly quoted ("$PR_NUMBER", "$BASE").
• // empty in jq correctly produces empty string (not null) when no PR exists.
• git rev-parse --verify --quiet safely checks branch existence without error output.
• Step outputs via $GITHUB_OUTPUT (modern GitHub Actions pattern, not deprecated set-output).

ℹ️ Why the git identity is required

The old workflow never created commits — it only did checkout -B and push. The new workflow creates merge commits (git merge --no-edit -m "...") and git requires an author identity for that. The github-actions[bot] user with email 41898282+github-actions[bot]@users.noreply.github.com is the standard GitHub Actions bot identity (41898282 is the bot's numeric user ID). This is the correct and conventional approach.

✅ Consumer Impact

This change modifies an internal CI workflow only. No public API surfaces, exported packages, or schemas are affected. No downstream consumer impact.

Verdict: APPROVE ✅

No P0 or P1 issues found. The fix is well-designed, handles edge cases properly, and includes multiple safety improvements over the original (--force-with-lease, concurrency group, conflict detection, no-op skip). The PR description is thorough and accurately explains both the problem and the solution.

Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack

Re-review: fix: sync-main-to-experimental action correct base branch

Files changed: 2 — .github/workflows/sync-main-to-experimental.yml (+83 −6), .github/workflows/merge-sync-to-experimental.yml (new, +67)
CI: Pending (check_and_build, Validate compatibility — both in progress)
ADR-6: PR title (fix: …) and branch (fix/sync-main-to-experimental-action) follow semantic conventions ✅
Head: a364e95 (includes "mini refactor based on PR feedback")

What changed since the last review (5ece7fe → a364e95)

The latest commit addresses the two code-level P1s from @charly-bg's review:

Additional cleanup: issues: write removed, --no-edit removed, --set-upstream removed, --force-with-lease claim dropped, branch-reuse decoupled from PR existence (just checks branch existence).

Analysis of current state

✅ Core merge logic

The fundamental fix is correct: branching from origin/experimental (or reusing origin/chore/sync) and merging origin/main into it preserves experimental-only content. The for-loop approach handles both fresh and reuse paths uniformly.

✅ Concurrency & race protection

• concurrency groups with cancel-in-progress: false serialize pushes to main correctly.
• --force-with-lease correctly rejects the push if a human pushed to chore/sync between fetch and push (verified: the tracking ref from the fetch is the lease comparand).

✅ Push & PR gate separation

• Push decision: HEAD vs REMOTE_HEAD — pushes whenever the recomputed branch differs from the remote. This fixes charly's scenario (main reverts → stale remote stays). ✅
• PR-create decision: merge-base --is-ancestor HEAD origin/experimental — skips PR creation when experimental already contains everything. ✅

✅ Security

• No injection vectors: All dynamic values are either hardcoded strings or integers from gh API. No untrusted input flows into shell commands.
• Permissions minimal: contents: write + pull-requests: write (sync), contents: write + pull-requests: read (merge). All justified.
• Triggers safe: push on main (requires repo write access) and workflow_dispatch (same). No pull_request_target or other external-triggerable events.
• No secrets exposure.

✅ Conflict handling

merge --abort || true + expanded ::error:: with four recovery steps + exit 1. Clean, nothing pushed on conflict. The recovery instructions correctly cover both the "branch exists" and "branch doesn't exist" cases.

Remaining findings (all P2)

[P2] Merge workflow no-op path skips cleanup

merge-sync-to-experimental.yml: when --is-ancestor origin/chore/sync origin/experimental is true (already merged), the workflow exits 0 without deleting chore/sync or affecting the PR. This is reachable after a partial failure (experimental push succeeds, branch delete fails in a prior run) or an external merge. Re-running the merge workflow always hits the same no-op exit.

Self-heals on next meaningful push to main (sync workflow recomputes and pushes). Manual fix is trivial (git push origin --delete chore/sync). Consider adding git push origin --delete chore/sync || true to the no-op path for robustness.

[P2] Redundant git config --replace-all in both workflows

With fetch-depth: 0, actions/checkout@v4 already sets remote.origin.fetch to the wildcard refspec via getRefSpecForAllHistory. The git config --replace-all line is dead code (as charly noted). The git fetch that follows is useful (refreshes refs), but the config line can be removed. Harmless, but noise.

[P2] Empty-diff PR lingers after net-zero main changes

If main adds then reverts a change while a sync PR is open, chore/sync accumulates merge commits not in experimental's graph even though the tree is identical. --is-ancestor (commit-graph-based) returns false, so the PR stays open with 0 files changed. Merging it creates an empty merge commit on experimental — harmless but noisy. A supplementary git diff --quiet origin/experimental HEAD check could catch this and skip PR maintenance.

Verdict: APPROVE ✅

No P0 or P1 issues. The latest commit substantively addresses both code-level P1s from the prior review. The push/PR gate separation is correct, conflict recovery is well-documented, and the two-workflow design (sync creates the branch, merge lands it as a real merge commit) is sound for working around the squash-only constraint. The P2 items above are worth addressing but do not block merge.

Note: charly's third P1 (squash-only repo setting) is a precondition — worth confirming merge commits are re-enabled or that the team is committed to using the merge workflow exclusively, but not a defect in this diff.

Reviewed by Jarvis 🤖 · Requested by Pravus (<@UDJQDQC0Z>) via Slack

Re-review (round 3): fix: sync-main-to-experimental action correct base branch

Head: d80fdb7 ("tackled review feedback")
CI: All checks passing ✅ (check_and_build, Validate compatibility, Deployment Notification)
ADR-6: ✅

Changes since last review (a364e95 → d80fdb7)

All three P2 findings from my prior review are addressed:

Verification of new logic

Merge workflow cleanup (L46-48):

if git merge-base --is-ancestor origin/chore/sync origin/experimental; then
    echo "experimental already contains chore/sync; nothing to merge."
    git push origin --delete chore/sync || true
    exit 0
fi

Correct. The || true handles the case where the branch is already gone. Recovers from partial failures (prior run pushed experimental but failed on branch delete).

Sync workflow tree check (L94-98):

if git merge-base --is-ancestor HEAD origin/experimental \
   || git diff --quiet origin/experimental HEAD; then
    echo "experimental already has everything chore/sync would propose."
    exit 0
fi

Correct. First condition catches commit-graph containment. Second condition catches tree-identical-but-different-history (the net-zero revert scenario). || means either skips PR creation/update. The push step still runs independently, so the remote chore/sync branch is always up to date.

git config --replace-all removal: Both workflows now rely on actions/checkout@v4 with fetch-depth: 0 to set the wildcard refspec, followed by git fetch --no-tags --force --prune origin to refresh refs. Correct — the explicit config was redundant.

Verdict: APPROVE ✅

No P0 or P1 issues. All prior P2s resolved. CI green. The workflow logic is sound:

• Push gate (HEAD vs remote chore/sync) ensures the remote branch is always current
• PR-create gate (commit-graph + tree-diff) avoids both empty PRs and gh pr create failures
• Merge workflow cleans up stale branches on both the happy path and the already-merged path
• Conflict recovery instructions are comprehensive
• Concurrency groups, --force-with-lease, and set -euo pipefail provide proper safety

Reviewed by Jarvis 🤖 · Requested by Pravus (<@UDJQDQC0Z>) via Slack